Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Search highlighting for Collectives #1368

Merged
merged 15 commits into from
Jul 25, 2024
Merged

Conversation

elzody
Copy link
Contributor

@elzody elzody commented Jul 15, 2024

📝 Summary

This pull request integrates search highlighting from the text app into collectives.

🖼️ Video

search_highlighting_example1.mp4

🚧 TODO

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@elzody elzody self-assigned this Jul 15, 2024
@elzody elzody linked an issue Jul 15, 2024 that may be closed by this pull request
Copy link

cypress bot commented Jul 15, 2024

2 failed and 2 flaky tests on run #1739 ↗︎

2 222 0 0 Flakiness 2

Details:

feat: Search highlighting for Collectives
Project: Collectives Commit: 02fb6d23db
Status: Failed Duration: 14:17 💡
Started: Jul 25, 2024 4:24 PM Ended: Jul 25, 2024 4:38 PM
Failed  pages.spec.js • 1 failed test • Nextcloud master

View Output

Test Artifacts
Pages > Creating a page from template > New page has template content Screenshots
Failed  page-list.spec.js • 1 failed test • Nextcloud master

View Output

Test Artifacts
Page list > Page trash > allows to trash and restore page with subpage and attachment Screenshots
Flakiness  cypress/e2e/collective-settings.spec.js • 2 flaky tests • Nextcloud stable27

View Output

Test Artifacts
Collective settings > rename collective > Allows to rename the collective Screenshots
Collective settings > open settings from landing page actions > Allows to open settings from landing page actions Screenshots

Review all test suite changes for PR #1368 ↗︎

@nimishavijay

This comment was marked as resolved.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Only additional feedback in addition to @nimishavijay’s is:

  • The elements in the left navigation directly below the search field jump around vertically a little bit. It would be nice if they stay in place, e.g. the main Collective page.

@juliusknorr
Copy link
Member

Nice! Only additional feedback in addition to @nimishavijay’s is:

  • The elements in the left navigation directly below the search field jump around vertically a little bit. It would be nice if they stay in place, e.g. the main Collective page.

Let's file that into a separate issue. Unrelated to this part of the implementation

@elzody elzody marked this pull request as ready for review July 23, 2024 15:48
@jancborchardt jancborchardt dismissed their stale review July 23, 2024 16:06

Different issue

@elzody elzody enabled auto-merge (squash) July 23, 2024 17:17
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for working on this @elzody ❤️

It's working really well. I have a few comments, but they should be easy to address.

src/components/SearchDialog.vue Show resolved Hide resolved
src/components/SearchDialog.vue Outdated Show resolved Hide resolved
src/store/search.js Outdated Show resolved Hide resolved
src/store/search.js Outdated Show resolved Hide resolved
src/components/SearchDialog.vue Show resolved Hide resolved
src/components/SearchDialog.vue Outdated Show resolved Hide resolved
elzody and others added 6 commits July 25, 2024 18:02
Signed-off-by: Elizabeth Danzberger <[email protected]>
Signed-off-by: Elizabeth Danzberger <[email protected]>
Signed-off-by: Elizabeth Danzberger <[email protected]>
Signed-off-by: Elizabeth Danzberger <[email protected]>
* Consolidate vuex search module
* Get rid of watcher + duplicated state around `matchAll`/`highlightAll`
* Scroll current search result into view

Signed-off-by: Jonas <[email protected]>
@mejo- mejo- disabled auto-merge July 25, 2024 16:40
@mejo- mejo- merged commit 006385d into main Jul 25, 2024
46 of 49 checks passed
@mejo- mejo- deleted the feat/search-highlighting branch July 25, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Search highlight
6 participants